Skip to content

feat: enable on key in join_asof#916

Merged
MarcoGorelli merged 2 commits intonarwhals-dev:mainfrom
raisadz:add-on-key-joinasof
Sep 6, 2024
Merged

feat: enable on key in join_asof#916
MarcoGorelli merged 2 commits intonarwhals-dev:mainfrom
raisadz:add-on-key-joinasof

Conversation

@raisadz
Copy link
Copy Markdown
Contributor

@raisadz raisadz commented Sep 6, 2024

What type of PR is this? (check all applicable)

  • 💾 Refactor
  • ✨ Feature
  • 🐛 Bug Fix
  • 🔧 Optimization
  • 📝 Documentation
  • ✅ Test
  • 🐳 Other

Related issues

Checklist

  • Code follows style guide (ruff)
  • Tests added
  • Documented the changes

If you have comments or can explain your changes, please do so below.

@github-actions github-actions bot added the enhancement New feature or request label Sep 6, 2024
Copy link
Copy Markdown
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @raisadz , looks great! just one comment

Comment on lines +3579 to +3608
return super().join_asof(
other, left_on=left_on, right_on=right_on, strategy=strategy
)
if left_on is not None and right_on is not None and on is not None:
msg = "Either (`left_on` and `right_on`) or `on` keys should be specified."
raise ValueError(msg)
if left_on is not None and right_on is not None:
return super().join_asof(
other, left_on=left_on, right_on=right_on, strategy=strategy
)
elif on is not None:
return super().join_asof(other, on=on, strategy=strategy)
else:
msg = "Either (`left_on` and `right_on`) or `on` keys should be specified."
raise ValueError(msg)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does this need updating, or can you also just defer to super().join_asof( here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, updated this part

Copy link
Copy Markdown
Member

@MarcoGorelli MarcoGorelli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @raisadz !

@MarcoGorelli MarcoGorelli merged commit 4cf94ce into narwhals-dev:main Sep 6, 2024
@raisadz raisadz deleted the add-on-key-joinasof branch November 7, 2024 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants